Allow use of map for qualifiers in constructor #155
Conversation
dwalluck
force-pushed
the
allow-map-in-constructor
branch
3 times, most recently
from
0d0777e to
7c10d53
Compare
| public PackageURL(final String type, final String namespace, final String name, final String version, | ||
| final Map<String, String> qualifiers, final String subpath) | ||
| throws MalformedPackageURLException { | ||
| this(type, namespace, name, version, (qualifiers == null) ? null : ((qualifiers instanceof TreeMap) ? (TreeMap<String, String>) qualifiers : new TreeMap<>(qualifiers)), subpath); |
There was a problem hiding this comment.
| this(type, namespace, name, version, (qualifiers == null) ? null : ((qualifiers instanceof TreeMap) ? (TreeMap<String, String>) qualifiers : new TreeMap<>(qualifiers)), subpath); | |
| this(type, namespace, name, version, (qualifiers == null) ? null : new TreeMap<>(qualifiers), subpath); |
This will prevent the caller from unintentionally modifying the state of PackageURL by modifying the map.
Should this one be the canonical constructor that the one above should call?
There was a problem hiding this comment.
My version works to avoid making two copies of the map if the original constructor is making a copy, but it's not. I fixed it in #169 to parse the qualifiers and therefore make a copy.
The behavior of the existing code inconsistent. Right now, it's making a copy if you use the String constructor, but not making a copy if you use this constructor. I could close this and move this code to #169 in which case the way I have it written would make more sense, I think.
There was a problem hiding this comment.
Probably it would make sense to merge this after #169.
The `PackageURL` constructor should not require the use of `TreeMap` and can just convert the map to a `TreeMap` if it isn't already.
dwalluck
force-pushed
the
allow-map-in-constructor
branch
from
a27618b to
80cfd90
Compare
dwalluck
force-pushed
the
allow-map-in-constructor
branch
from
80cfd90 to
cfbffe8
Compare
3 participants
The
PackageURLconstructor should not require the use ofTreeMapand can just convert the map to a
TreeMapif it isn't already.